Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for executeScript/evaluateScript args #826

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Add support for executeScript/evaluateScript args #826

wants to merge 1 commit into from

Conversation

mvorisek
Copy link
Contributor

The args support is necessary as webdriver objects must be passed natively to the underlaying drivers & cannot be serialized directly into the 1st script argument.

Example executeScript method prototype of one of the most popular driver - https://github.com/php-webdriver/php-webdriver/blob/8ffa927b270e932449e8015abf4d38bb0eff24b7/lib/Remote/RemoteWebDriver.php#L324

@codecov
Copy link

codecov bot commented Apr 14, 2022 •
edited
Loading

Codecov Report

Merging #826 (b37c535) into master (19e5890) will decrease coverage by 0.29%.
The diff coverage is 75.00%.

@@             Coverage Diff              @@
##             master     #826      +/-   ##
============================================
- Coverage     98.47%   98.17%   -0.30%     
  Complexity      345      345              
============================================
  Files            23       24       +1     
  Lines           983      986       +3     
============================================
  Hits            968      968              
- Misses           15       18       +3
Impacted Files Coverage Δ
src/Driver/DriverInterface.php 0.00% <0.00%> (ø)
src/Driver/CoreDriver.php 100.00% <100.00%> (ø)
src/Session.php 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 19e5890...b37c535. Read the comment docs.

@mvorisek mvorisek force-pushed the allow_execute_args branch from f49f3a1 to d4424d1 Compare April 14, 2022 22:21
@aik099
Copy link
Member

aik099 commented Apr 15, 2022

Adding more parameters to the DriverInterface interface might be a BC break (not sure if an optional argument causes BC breaks). I guess you'll need to create PR for every driver to pass through these arguments. I guess the Selenium2 is the only one capable of it.

@mvorisek mvorisek marked this pull request as draft April 15, 2022 11:26
@mvorisek
Copy link
Contributor Author

Yes, it is fatal error BC break - https://3v4l.org/kO2hW.

I will create PRs to these drivers - https://github.com/minkphp/MinkSelenium2Driver and https://github.com/silverstripe/MinkFacebookWebDriver shortly. What are the other popular drivers?

@mvorisek mvorisek force-pushed the allow_execute_args branch from d4424d1 to c5f9552 Compare April 15, 2022 11:31
@stof
Copy link
Member

stof commented Apr 15, 2022

We simply cannot do such BC break in a minor version. That would mean we stop applying semver.

@mvorisek mvorisek force-pushed the allow_execute_args branch from c5f9552 to b37c535 Compare April 15, 2022 11:35
@mvorisek
Copy link
Contributor Author

Are there any objections to release a v2?

@mvorisek mvorisek mentioned this pull request Apr 15, 2022
Draft
@stof
Copy link
Member

stof commented Jun 9, 2023

even for doing a v2, we would want to provide a migration path

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mvorisek @aik099 @stof